Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for missing NAs in estimate_infection() model #528

Merged
merged 18 commits into from
Jan 9, 2024

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Dec 18, 2023

This PR adds support for missing data in the form of NAs but dropping them from the likelihood rather than imputing that they are zero and then optionally

Note that this is a breaking change and may be undesirable for some users where missing dates do indicate zeros vs being truly missing.

I have tested by knocking out data from the example and running the model.

@seabbs seabbs force-pushed the missing-data-likelihood branch 3 times, most recently from e904fe4 to 9647232 Compare December 21, 2023 16:38
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if cb5705c is merged into main:

  •   :ballot_box_with_check:default: 32.5s -> 32.2s [-18.26%, +16.05%]
  •   :ballot_box_with_check:no_delays: 33.5s -> 32.3s [-19.32%, +12.51%]
  •   :ballot_box_with_check:random_walk: 9.78s -> 9.97s [-5.78%, +9.75%]
  •   :ballot_box_with_check:stationary: 1.18m -> 20.6s [-232.86%, +91.02%]
  •   :ballot_box_with_check:uncertain: 50.1s -> 53.3s [-5.13%, +17.82%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! A couple of minor questions/confusions on my side.

NEWS.md Outdated Show resolved Hide resolved
R/create.R Show resolved Hide resolved
R/create.R Show resolved Hide resolved
R/create.R Outdated Show resolved Hide resolved
R/create.R Outdated Show resolved Hide resolved
R/create.R Outdated Show resolved Hide resolved
sbfnk
sbfnk previously approved these changes Jan 8, 2024
@seabbs

This comment was marked as outdated.

@seabbs

This comment was marked as outdated.

@seabbs seabbs requested a review from sbfnk January 8, 2024 21:59
sbfnk
sbfnk previously approved these changes Jan 8, 2024
Copy link
Contributor

github-actions bot commented Jan 8, 2024

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 876a533 is merged into main:

  • ❗🐌default: 33.6s -> 37.9s [+0.54%, +24.72%]
  •   :ballot_box_with_check:no_delays: 34.8s -> 37.3s [-9.79%, +24.19%]
  •   :ballot_box_with_check:random_walk: 10.5s -> 9.47s [-32.17%, +13.29%]
  •   :ballot_box_with_check:stationary: 20s -> 19.3s [-22.76%, +16.34%]
  •   :ballot_box_with_check:uncertain: 51s -> 48.5s [-25.08%, +15.29%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs seabbs merged commit 0a8569e into main Jan 9, 2024
@seabbs seabbs deleted the missing-data-likelihood branch January 9, 2024 09:29
sbfnk added a commit that referenced this pull request May 3, 2024
* add a lookup to estimate_infections

* add R side support

* don't internally impute missing as zero

* update news

* fix data preprocessing order

* correction data ingestion

* clean up filtering of leading zeros

* error check create_clean_reported_cases and add unit tests to cover function

* correct handling of missing data in data preprocessing:

* refine data preprocessing

* update news and tests

* update global variables

* Update NEWS.md

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* fix line length linting

* Document

---------

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: GitHub Actions <actions@github.com>
sbfnk added a commit that referenced this pull request May 3, 2024
* add a lookup to estimate_infections

* add R side support

* don't internally impute missing as zero

* update news

* fix data preprocessing order

* correction data ingestion

* clean up filtering of leading zeros

* error check create_clean_reported_cases and add unit tests to cover function

* correct handling of missing data in data preprocessing:

* refine data preprocessing

* update news and tests

* update global variables

* Update NEWS.md

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* Update R/create.R

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

* fix line length linting

* Document

---------

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: GitHub Actions <actions@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants